Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix : Paragraph appender layout shift #66061

Closed

Conversation

Vrishabhsk
Copy link
Contributor

What?

  • Adds a style that targets block-editor-default-block-appender__content placeholder
  • The style sets margin-block-start & margin-block-end to 0

Why?

const onAppend = () => {
	insertDefaultBlock( undefined, rootClientId );
	startTyping();
};

How?

  • The style added here ensure the layout of the content in the editor does not shift when onClick is triggered

@Vrishabhsk Vrishabhsk requested a review from ellatrix as a code owner October 11, 2024 12:44
Copy link

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core, Gutenberg Plugin.
  • Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link

github-actions bot commented Oct 11, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Vrishabhsk <vrishabhsk@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@jasmussen
Copy link
Contributor

Thanks for the PR. As an override, I imagine this can work, but I'd like to see if we can get to the root of why the rule to zero out margins exists for constrained layouts in the first place, and not outside of it. To that end, we may need to get closer to the layout engine. I pinged Aaron in case he knows, but it's possible he hasn't been involved with layout, in which case hopefully he can pass on the ping to someone else.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping @jasmussen and spinning up this PR @Vrishabhsk 👍

I'm no layout expert so take the following with a grain of salt. It might also pay to get @andrewserong and @tellthemachines (once they are back on deck) to weigh in.

With that said, here's what I found digging into this a little:

  • The paragraph element within the block list appender gets extraneous spacing due to user agent margins set by the browser.
  • As noted on the original issue and this PR, those margins are reset on a paragraph block leading to the layout shift issue
  • My suspicion is that these margins should be reset across the board so that whatever is being set by layouts take affect on the appender's wrapper.
  • The BlockListAppender isn't exported from the block editor package and only used within the BlockList component which should make the blanket margin reset safer.

but I'd like to see if we can get to the root of why the rule to zero out margins exists for constrained layouts in the first place, and not outside of it.


In terms of this PR, the selector for the proposed styles has a very high specificity (0-4-1) and could be simplified a bit.

There are a couple of options:

1. Simply reset the paragraph margins within the block list appender.
diff --git a/packages/block-editor/src/components/default-block-appender/content.scss b/packages/block-editor/src/components/default-block-appender/content.scss
index 51e0b4381a..c82a04e8c7 100644
--- a/packages/block-editor/src/components/default-block-appender/content.scss
+++ b/packages/block-editor/src/components/default-block-appender/content.scss
@@ -24,20 +24,11 @@
 	.block-editor-default-block-appender__content {
 		// Set the opacity of the initial block appender to the same as placeholder text in an empty Paragraph block.
 		opacity: 0.62;
-	}
-
-	// In "constrained" layout containers, the first and last paragraphs have their margins zeroed out.
-	// In the case of this appender, it needs to apply those same rules to avoid layout shifts.
-	// Such shifts happen when the bottom margin of the Title block has been set to less than the default 1em margin of paragraphs.
-	:where(body .is-layout-constrained) &,
-	:where(.wp-site-blocks) & {
-		> :first-child {
-			margin-block-start: 0;
-			margin-block-end: 0;
-		}
 
-		// Since this appender will only ever appear on an entirely empty document, we don't account for last-child.
-		// This is also because it will never be the last child, the block inserter that sits in this appender is the last child.
+		// The following prevent user agent styles from applying margins to the appender's inner paragraph.
+		// This in turn prevents layout shift do to layout styles removing margins from first and last children.
+		margin-block-start: 0;
+		margin-block-end: 0;
 	}
 
 	// Dropzone.

2. Reset the paragraph margins when the appender is within any block with a class matching `is-layout-*`
diff --git a/packages/block-editor/src/components/default-block-appender/content.scss b/packages/block-editor/src/components/default-block-appender/content.scss
index 51e0b4381a..384c0986b3 100644
--- a/packages/block-editor/src/components/default-block-appender/content.scss
+++ b/packages/block-editor/src/components/default-block-appender/content.scss
@@ -29,7 +29,7 @@
 	// In "constrained" layout containers, the first and last paragraphs have their margins zeroed out.
 	// In the case of this appender, it needs to apply those same rules to avoid layout shifts.
 	// Such shifts happen when the bottom margin of the Title block has been set to less than the default 1em margin of paragraphs.
-	:where(body .is-layout-constrained) &,
+	:where(body [class*="is-layout-"]) &,
 	:where(.wp-site-blocks) & {
 		> :first-child {
 			margin-block-start: 0;

Both of the options above applied to trunk resolve the layout shift for me. I didn't spot any regressions from those changes but have only done some basic smoke testing.

I hope that helps!

@Vrishabhsk
Copy link
Contributor Author

Thanks @aaronrobertshaw

  • After running a search, I found that block-editor-default-block-appender__content class was set only in the default-block-appender
  • I have implemented the first suggestion

Simply reset the paragraph margins within the block list appender.

@andrewserong
Copy link
Contributor

Thanks for the ping!

My suspicion is that these margins should be reset across the board so that whatever is being set by layouts take affect on the appender's wrapper.

That's my suspicion, too. The way the default appender appears to work is that it's a paragraph element wrapped in a div, and that paragraph is rendered to look like a paragraph block. This means that the styling needs to match what a paragraph block would look like within a container, and the way each of the layouts work is that the first block within a container has its top margin set to 0, even if that block has its own margin styles in global styles. So overall the idea of applying this in all cases sounds good to me 👍

I suppose a potential edge case could be a classic theme that doesn't expect the styles to work in this way. But even then, for Gutenberg to provide a default for this component that it sets margins to 0 sounds reasonable to me.

This is working well in testing. I notice that the rule directly after it applies to the first child (the rules that currently target is-layout-constrained). As block-editor-default-block-appender__content is the first child, do we still need that set of rules if we're going with always applying the 0 margin?

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my suspicion, too.

Thanks for the confirmation @andrewserong 👍

do we still need that set of rules if we're going with always applying the 0 margin?

I don't think we do. That's why I removed them in the snippets I floated previously. Looks like it was just missed that's all.

I suppose a potential edge case could be a classic theme that doesn't expect the styles to work in this way.

Yeah, I wasn't too sure how far to follow that thread. After tweaking styles locally, I only had a quick smoke test of the usual classic theme suspects like TT1 etc. They seemed ok to me.

Click clip of TT1 appender
Screen.Recording.2024-10-15.at.5.34.19.pm.mp4

But even then, for Gutenberg to provide a default for this component that it sets margins to 0 sounds reasonable to me.

Agreed.

@andrewserong
Copy link
Contributor

I was in the process of giving this a rebase but noticed that in trunk there's a regression for the position of the appender. I've left a comment on the PR where it appears to have been introduced #66630 (review) — a may not get a chance to work on a fix before the end of the week, though. IMO that regression will need to be resolved before we attempt to land this one.

@andrewserong
Copy link
Contributor

Now that the fix in #66711 has been merged, I attempted to give this PR a rebase and update, but for some reason I'm unable to push to this branch.

I hope you don't mind @Vrishabhsk, but I've copied your work here over into a fresh PR in #66779, incorporating some of the feedback from @aaronrobertshaw in #66061 (review).

Thanks again for all your work here!

@getdave
Copy link
Contributor

getdave commented Nov 6, 2024

Now #66779 has merged can this be closed out?

@andrewserong
Copy link
Contributor

Yes, I'll close this out now. Thanks again for the fix @Vrishabhsk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants